fix(cli): probe dev ports without the dnt Deno shim - #3599
Conversation
`veryfront dev` died before binding anything whenever the CLI ran on Deno
(`deno install -g npm:veryfront`, `deno run -A npm:veryfront dev`, or the
`deno task dev` a `--runtime deno` scaffold prints):
✗ [unknown-error] Unknown/unclassified error
Detail: Cannot read properties of null (reading 'fd')
The port probe branched on `isDeno` and then called the bare `Deno.listen`.
In the npm build dnt rewrites that bare global to `@deno/shim-deno`, whose
Node-backed `listen()` reads `server._handle.fd` synchronously - and under
Deno's node compat `_handle` is still null at that point. So the one branch
written for Deno was the one branch Deno could not run, and `dev` was the
only command that reached it.
Probe with `node:net` on every runtime instead. It is the path this function
already used on Node, it behaves identically on Deno (source, npm build and
`deno compile` binary all verified), and it cannot reach the shim.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesPort probing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/commands/dev/port-fallback.ts (1)
38-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce duplicated implementation-detail comments.
Keep the runtime reason concise. Move detailed shim failure mechanics to the PR description or an issue if they need long-term retention.
cli/commands/dev/port-fallback.ts#L38-L44: reduce the JSDoc to the reason thatnode:netis used on all runtimes.cli/commands/dev/port-fallback.test.ts#L130-L136: reduce the test comment to the mocked condition and expected behavior.As per coding guidelines, use “direct, concise, active, present-tense public language.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/commands/dev/port-fallback.ts` around lines 38 - 44, Shorten the JSDoc in cli/commands/dev/port-fallback.ts:38-44 to state concisely why node:net is used across all runtimes, removing detailed shim failure mechanics. Also shorten the test comment in cli/commands/dev/port-fallback.test.ts:130-136 to describe only the mocked condition and expected behavior, using direct, concise, active, present-tense language.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/commands/dev/port-fallback.ts`:
- Around line 38-44: Shorten the JSDoc in
cli/commands/dev/port-fallback.ts:38-44 to state concisely why node:net is used
across all runtimes, removing detailed shim failure mechanics. Also shorten the
test comment in cli/commands/dev/port-fallback.test.ts:130-136 to describe only
the mocked condition and expected behavior, using direct, concise, active,
present-tense language.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c75453a-cf63-4423-bc10-dd9dc3e10260
📒 Files selected for processing (2)
cli/commands/dev/port-fallback.test.tscli/commands/dev/port-fallback.ts
Carries over the static invariant test from the duplicate PR #3598 alongside the behavioural one already here. They catch different things: - the behavioural test poisons the ambient `Deno.listen` and asserts the probe still answers correctly, proving the fix works when the shim is in place; - this one reads the source and asserts no bare `Deno.<member>` access survives anywhere in the file, catching a future reintroduction on paths the behavioural test never executes. Verified red both ways before landing: against the pre-fix source both tests fail, and with a bare `Deno.hostname()` planted in `findAvailablePort`'s throw path the behavioural test passes while this one fails naming `Deno.hostname`. Comments are stripped before matching, since dnt rewrites code and not prose, and `isPortAvailable`'s doc comment has to stay free to name `Deno.listen` as the call the fix removed.
Review nitpick. Keeps what a future reader cannot recover from the code - that dnt swaps the ambient `Deno` for a shim whose `listen` reads a null `server._handle` - and drops the retelling of the incident around it.
|
Addressed the nitpick in 7b9f2ce: both comments trimmed to the part a reader cannot recover from the code — dnt swaps the ambient |
|
Closing as superseded by #3610, which merged at These were competing implementations of one fix, not two fixes:
Rebasing this onto main is not a conflict resolution but an adjudication between the two, so it was left alone rather than decided unilaterally. A trap worth recording
So a rebase that resolved only the Nothing is lost by closingThe static source-level guard this PR carried (ported from #3598) has an equivalent on main — If the single-path Branch left in place. |
kwakayama
left a comment
There was a problem hiding this comment.
Found one low-severity documentation issue.
- [P3] Update the stale remediation guidance in
cli/commands/dev/port-fallback.test.ts:174. It says the safe approach isgetDenoRuntime()/Reflect.get(globalThis, "Deno"), but the implementation now deliberately usesimport("node:net")incli/commands/dev/port-fallback.ts:43. This is misleading to future maintainers and could steer a regression fix toward a different, untested path.
Rubric:
- Correctness: 40/40
- Tests: 20/20
- Reliability/security: 15/15
- Maintainability: 13/15
- Scope/docs: 8/10
Review-Gate:
Reviewer: Codex
Reviewed-SHA: 7b9f2ce
Score: 96/100
Actionable-Findings: 1
Verdict: COMMENT
The symptom
On published
0.1.1229,veryfront devnever binds a port when the CLI runs on Deno — the Deno-global install (deno install -gArf npm:veryfront),deno run -A npm:veryfront dev, and thedeno task devthat a--runtime denoscaffold prints as its own next step:build,serve,routesanddoctorall succeed under the same install.devwas the only command that reached the broken code, and the npm-global (Node) install of the identical version runsdevfine.Root cause
The real stack, recovered by calling
handleDevCommanddirectly under Deno against the published tarball:isPortAvailablebranched onisDenoand then called the bareDeno.listen. In the npm build dnt rewrites every bareDenoto@deno/shim-deno—esm/_dnt.shims.jsre-exports the shim unconditionally, so the shim wins even when a genuineDenoglobal is right there. That shim's Node-backedlisten()does:server._handleis assigned synchronously by Node'slisten(), but not by Deno's node-compat layer, so it is stillnullon that line. The one branch written for Deno was the one branch Deno could not execute.The repo already knows this hazard —
cli/auth/callback-server.tsreaches the real namespace viaself("Access native Deno.serve viaselfto bypass dnt shim transform"), andgetNativeDeno()insrc/platform/compat/http/native-response.tsdocuments it.port-fallback.ts, which was written to copy that callback server's fallback, missed it.The fix
Probe with
node:neton every runtime and delete the Deno branch. This function already usednode:neton Node; it is one path now, it behaves identically on Deno, and it cannot reach the shim. Verified on all four runtime shapes the CLI ships in: Deno source,deno compilebinary, npm-on-Node, npm-on-Deno.Regression test
cli/commands/dev/port-fallback.test.tsstands a throwingDeno.listeninto the ambient namespace — the shim reproduced in-process, sameTypeError: Cannot read properties of null (reading 'fd')— and asserts the probe still reports a held port busy and a free port free. It fails onmainatport-fallback.ts:41with exactly the published error, and passes with this change.Proof against the published artifact, not just the tests
The finding's own command, re-run against
veryfront@0.1.1229with only this function transplanted into the installed tree:deno run -A ./node_modules/veryfront/bin/veryfront.js dev --port 3772✗ [unknown-error] … reading 'fd', nothing listening✓ Ready in 1.6s/http://veryfront.me:3772,lsofconfirmsTCP 127.0.0.1:3772 (LISTEN)! Port 3772 is in use, using 3773 instead→✓ Readynode ./node_modules/veryfront/bin/veryfront.js dev --port 3774(control)✓ Ready,curl200✓ Ready,curl200What this does not fix
With the port crash gone,
veryfront devunder Deno binds and reports Ready, but page renders still fail with a separate Deno defect: the SSR module loader dynamic-importing generated.cache/veryfront-http-bundle/http-*.mjsthrowsTypeError: Loading unprepared module: …, so/returns 500 (Node returns 200 on the identical project). That is a different failure in a different subsystem and belongs to the open "CLI is entirely non-functional under the Deno runtime" reports — deliberately out of scope here rather than folded into this change.Also from the originating finding, and deliberately unchanged: the published package declaring 7 of the 20
@veryfront/ext-*specifiers its runtime references is policy, not a packaging bug —rootNpminsrc/extensions/first-party-defaults.tskeeps credentialed, native and output-changing integrations out of the root dependency set, andfirst-party-import.tsnow degrades with an install hint instead of dying. No live doc page needs to change for this PR.Summary by CodeRabbit
Bug Fixes
Tests